fix(dav): finalize upload metadata before post-write hooks#60453
fix(dav): finalize upload metadata before post-write hooks#60453joshtrichards wants to merge 7 commits into
Conversation
Signed-off-by: Josh <josh.t.richards@gmail.com>
|
/backport to stable33 |
joshtrichards
requested review from
ArtificialOwl,
CarlSchwan,
icewind1991 and
leftybournes
and removed request for
a team
joshtrichards
added
3. to review
…mproving consistency Signed-off-by: Josh <josh.t.richards@gmail.com>
Signed-off-by: Josh <josh.t.richards@gmail.com>
joshtrichards
changed the title
joshtrichards
changed the title
At least where likely to be needed. Also fixed object type. Signed-off-by: Josh <josh.t.richards@gmail.com>
joshtrichards
added
2. developing
- `checksum` is already optional/derived metadata in practice - callers already treat `null`l / `''` as "no checksum" Signed-off-by: Josh <josh.t.richards@gmail.com>
Signed-off-by: Josh <josh.t.richards@gmail.com>
Signed-off-by: Josh <josh.t.richards@gmail.com>
joshtrichards
changed the title
|
Ready for review? I'd love to have stable CI again 🙏 |
miaulalala
left a comment
miaulalala
left a comment
There was a problem hiding this comment.
Nice fix — the core insight (keep the exclusive lock until all metadata is persisted) is correct, and the consolidation has a pleasant side-effect: checksum is now written in the same putFileInfo() call that writes mtime/ctime, so refreshInfo() finally reads the correct checksum. Previously setChecksum() ran after refreshInfo(), meaning the in-memory $this->fileView had a stale/empty checksum until the next request.
A few small observations:
getChecksum() !== null is now unreachable (nit)
FileInfo::getChecksum() now returns $this->data['checksum'] ?? '' (string, never null), so the null-guard in File.php can't fire. Worth removing or simplifying to !== ''.
touch() now runs under the exclusive lock
Previously the mtime/ctime writes happened after the lock was downgraded to shared. They now happen before, so they're serialised under the exclusive lock. That's strictly safer, just worth noting in case someone audits locking behaviour later.
$ctimeHeader !== '' is an improvement
The old truthy check would have rejected '0' as a valid ctime header. The explicit !== '' is more correct.
Test fixes look good
The TYPE_FOLDER → TYPE_FILE change and the unrelated testPutLargeFile fix are pre-existing bugs, good to clean up here.
Missing concurrency test
Totally understood that a reliable test for the race is hard to write. A comment in the code pointing to the issue number would help future readers understand why the lock is held longer.
joshtrichards
added
3. to review
2 participants
Summary
Tighten the DAV PUT finalization sequence in
apps/dav/lib/Connector/Sabre/File.phpso post-write hooks observe a more fully finalized file state, while preserving historical shared-lock behavior during hook execution.What changed
finalizeUpload()putFileInfo()call (previously a separate call after hooks)Why
Previously the lock downgraded too early — before checksum and metadata were fully persisted — creating a window where the file was visible at the final path with incomplete bookkeeping state. This is a likely cause of intermittent FilesDrop CI failures:
{"message":"Cannot set extra headers for non-existing file 'files/jHybAbajiZcXgfy/Alice/folder (2)'"} {"message":"Could not open file: drop/Alice/folder/a.txt (214), file does seem to exist"}Cc: @icewind1991 — may be related to the logging work in #56166.
Locking sequence after this change
This also makes the direct PUT path more consistent with
ChunkingV2Plugin.Scope
Only changes ordering during the final publish/finalization phase. Does not alter the part-file strategy, cross-storage move behavior, stream write path, or general hook semantics.
Review focus
post_writeputFileInfo()Checklist
3. to review, feature component)stable32)AI (if applicable)